perf: vectorize integer-to-decimal cast#4939
Conversation
andygrove
changed the title
6da4ddf to
94b9a45
Compare
andygrove
changed the title
cast_int_to_decimal128_internal used a per-element Decimal128Builder loop with a null check and branch per row. Replace it with a single vectorized unary_opt pass: a value that overflows the multiply or does not fit the output precision maps to null, and the input null buffer is carried over. ANSI mode must raise on out-of-range values rather than nulling them. unary_opt only nulls non-null inputs that overflow, so a null count beyond the input's signals an overflow; that O(1) check gates a rare element-wise rescan that reports the first offending value with Spark's exact error. This is a single pass for every eval mode and shape, so all shapes are faster (28-50%) with no regression, including the overflow case. Add unit tests for the legacy null-on-overflow, ANSI no-overflow, and ANSI overflow-error paths, plus a criterion benchmark. Part of apache#4936.
94b9a45 to
d62d695
Compare
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
| // Single vectorized pass: a value that overflows the multiply or does not fit the output | ||
| // precision maps to null. `unary_opt` only applies the closure to non-null slots and carries | ||
| // the input null buffer over, replacing the per-element builder loop without a second pass. | ||
| let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v| { |
There was a problem hiding this comment.
native/spark-expr/src/conversion_funcs/numeric.rs: the fast path closure computes v.checked_mul(multiplier).filter(|scaled| is_validate_decimal_precision(*scaled, precision)), and the ANSI rescan recomputes the same thing inverted as checked_mul(multiplier).map(|scaled| !is_validate_decimal_precision(...)).unwrap_or(true). Two spellings of one predicate invite drift where one is updated and the other is not.
Suggested change: extract a small local closure or helper, for example:
let fits = |v: i128| -> Option<i128> {
v.checked_mul(multiplier)
.filter(|scaled| is_validate_decimal_precision(*scaled, precision))
};
let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v| fits(v.into()));
// rescan:
if fits(v).is_none() { return Err(SparkError::NumericValueOutOfRange { .. }); }There was a problem hiding this comment.
Extracted a fits closure that is called from both the vectorized pass and the ANSI rescan, so there is one spelling of the predicate.
…utdated comment Extracts the 'multiply-then-fits-target-precision' predicate into a single 'fits' closure so the vectorized pass and the ANSI rescan cannot drift out of sync. Adds test_cast_int_to_decimal128_overflow_try_nulls asserting EvalMode::Try shares the null-on-overflow branch. Replaces the outdated 'sentinel + masking path' comment on the Legacy test with an accurate description of the null-on-overflow path.
There was a problem hiding this comment.
Thanks @andygrove, all three earlier threads are resolved: the fits closure is now the single spelling of the predicate (numeric.rs:670-673, used by both unary_opt and the ANSI rescan), the stale sentinel/masking comment is gone, and test_cast_int_to_decimal128_overflow_try_nulls pins the EvalMode::Try arm.
The refactor reads well. The ANSI path pays nothing extra in the common case: the overflow check is an O(1) null_count() comparison and the element-wise rescan only runs on the error path.
| } | ||
|
|
||
| #[test] | ||
| fn test_cast_int_to_decimal128_overflow_ansi_errors() { |
There was a problem hiding this comment.
test_cast_int_to_decimal128_overflow_ansi_errors asserts only result.is_err(). The rescan comment promises it "reports the first offending value with Spark's exact error," but nothing tests that. A refactor that scanned in reverse, or returned a different error variant, would still pass is_err(). Match on the variant and the offending value so that behavior is pinned:
let err = result.unwrap_err();
assert!(
matches!(
err,
SparkError::NumericValueOutOfRange { ref value, precision: 3, scale: 2 } if value == "1000"
),
"unexpected error: {err:?}"
);Adding a second overflowing value ahead of 1000 in the input and keeping the assertion on 1000 would also pin the "first offending value" part of the promise, since that is the row the rescan is documented to report.
Requesting changes to fold this in on the same push, since the branch has merge conflicts to resolve anyway.
2 participants
Which issue does this PR close?
Part of #4936.
Rationale for this change
CAST(<int> AS DECIMAL(p, s))is common in TPC-DS (decimal casts are the second most frequent cast afterAS DATE). The native path,cast_int_to_decimal128_internal, used a per-elementDecimal128Builderloop with a null check and branch on every row.What changes are included in this PR?
Replaces the element-wise loop with a single vectorized
unary_optpass: a value that overflows the multiply or does not fit the output precision maps to null, and the input null buffer is carried over.unary_optapplies the closure only to non-null slots, so it reproduces the loop's behavior in one pass with no sentinel and no second masking pass.ANSI mode must raise on out-of-range values instead of nulling them.
unary_optonly nulls non-null inputs that overflow, so a null count beyond the input's signals an overflow. That check is O(1) and gates a rare element-wise rescan that reports the first offending value with Spark's exactNumericValueOutOfRangeerror.How are these changes tested?
Added unit tests for the legacy null-on-overflow (with nulls preserved), ANSI no-overflow, and ANSI overflow-error paths; the existing no-overflow test still passes. Output is bit-identical to
main.Because it is a single pass for every eval mode and shape, there is no regression on any shape: